Skip to content

Always traverse Inlined.call in linter #24043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Oct 8, 2025
Merged

Always traverse Inlined.call in linter #24043

merged 4 commits into from
Oct 8, 2025

Conversation

som-snytt
Copy link
Contributor

@som-snytt som-snytt commented Sep 23, 2025 •
edited
Loading

Previously it incorrectly only looked at call after typer, but it must also look at call after inlining, for intermediate expansions which are not directly written by user.

Fixes #24034

@WojciechMazur
Copy link
Contributor

I wonder if there is something missing before we can merge that? I'd like to backport this change to 3.7.4-RC2 as it would make the semi-automatic migrations under -rewrite -source:3.x-migration more stable (we relay on that in the Open Community Build). We might want to have RC2 someday next week

@som-snytt
Copy link
Contributor Author

som-snytt commented Oct 2, 2025 •
edited
Loading

I'll mark it ready today, as I have more experience with inlined positions.

Edit: So akshually, when reporting, it builds the outer chain for inlined positions from the context, which knows about enclosingInlineds, a mechanism which comes for free with miniphase dispatch.

It seems to be more precise than the mechanism in the linter. Moreover, the only position test is whether the "outermost" (last of enclosingInlineds) contains the current tree.

@som-snytt
Always traverse Inlined.call in linter
f7fd77d 
Previously it incorrectly only looked at call after typer,
but it must also look at call after inlining, for intermediate
expansions which are not directly written by user.
@som-snytt
Use enclosingInlineds
685a4e1
@som-snytt
Register expansion def for lint if in user code
a536a1b
@som-snytt
Copy link
Contributor Author

For reference, the inlined call and bindings

                                            Inlined(
                                              Apply(
                                                Apply(
                                                  TypeApply(
                                                    Ident(summonEncoder), List(
                                                    TypeTree())),
                                                List(Literal(^[[32mfalse^[[0m))),
                                              List(
                                                Select(
                                                  Inlined(Thicket(List()),
                                                    List(),
                                                    Ident(
                                                      EncoderNotDeriveSum_this)
                                                  ),
                                                  io$circe$derivation$EncoderNotDeriveSum$$inline$config
                                                  )
                                              )),
                                              List(
                                                ValDef(x$2$proxy1, TypeTree(),
                                                  Select(
                                                    Inlined(Thicket(List()),
                                                      List(),
                                                      Ident(
                                                        EncoderNotDeriveSum_this
                                                        )
                                                    ),
                                                    io$circe$derivation$EncoderNotDeriveSum$$inline$config
                                                    )
                                                )
                                              ),

and expansion

                                              Typed(
                                                Block(
                                                  List(
                                                    ValDef(encodeA, TypeTree(),
                                                      Inlined(
                                                        Apply(
                                                          TypeApply(Ident(XXX),
                                                            List(TypeTree(),
                                                            TypeTree())),
                                                        List(
                                                          Ident(
                                                            given_Encoder_String
                                                            )
                                                        )),
                                                      List(),
                                                        Typed(Ident(???),
                                                          AppliedTypeTree(
                                                            Ident(Encoder),
                                                            List(
                                                            AppliedTypeTree(
                                                              Ident(:|), List(
                                                              TypeTree(),
                                                              TypeTree()))
                                                          ))
                                                        )
                                                      )
                                                    )
                                                  ),
                                                  Typed(Ident(encodeA),
                                                    TypeTree())
                                                ),
                                                AppliedTypeTree(Ident(Encoder),
                                                  List(TypeTree()))
                                              )
                                            ),

where the XXX must be looked up to mark an import "used"; that happens because isZeroExtentSynthetic, due to summonFrom. (It has nothing to do with enclosing inlined positions.)

but encodeA is not registered for checking, presumably it is not looked up. I think that restriction is so that imports in enclosing contexts are not incorrectly marked used by inlined references; but the balance struck here seems dubious to me.

Perhaps there would be greater fidelity by checking everything and only restricting import lookups to isZES.

@som-snytt som-snytt force-pushed the issue/24034-given-import branch from 891100e to a536a1b Compare October 3, 2025 01:28
@som-snytt som-snytt marked this pull request as ready for review October 3, 2025 17:04
@som-snytt
Copy link
Contributor Author

If the PR is still open, I'll try the improvement from the last comment, something about check all code but restrict import lookups to summonInline or similar, as detected by position. Maybe also implement -Winline as a flag whether to warn in expansions: it's not obvious that such a flag would be as useful as -Wmacros in Scala 2.

@som-snytt
Copy link
Contributor Author

som-snytt commented Oct 3, 2025 •
edited
Loading

Finally edited the rooted imports inherited from previous code. It would be nice if we got improved syntax.

Always resolve, but pass a flag to resolveUsage that indicates whether to resolve imports. Do resolve imports for code which might require imports, namely, summonInline and its ilk, as well as user written code.

@Gedochao Gedochao requested a review from tgodzik October 8, 2025 09:00
tgodzik
tgodzik approved these changes Oct 8, 2025
View reviewed changes
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@som-snytt
Copy link
Contributor Author

Obsolete comment:

I added names to the iron givens to make it easier to find them in -Yplain-printer output; XXX was deeply nested in an Inlined tree. The restriction on phaseMode was due to a misunderstanding about inlining.

The mechanism of a stack of inlined positions, used to control name look-ups, needs further research or investigation. It's needed for the commented existing test. Speculating that the useful distinction is between the call of a top-most Inlined (normal code) and anything else (expansions and nested inlining); and the existing heuristic for summonInline (which performs resolution at expansion).

@som-snytt som-snytt merged commit eb67a9c into scala:main Oct 8, 2025
49 checks passed
@som-snytt som-snytt deleted the issue/24034-given-import branch October 8, 2025 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tgodzik tgodzik tgodzik approved these changes

Assignees

@tgodzik tgodzik

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

False positive unused given for circe / iron combination

3 participants

@som-snytt @WojciechMazur @tgodzik